test(xsnap): Verify npm install build process #11051

kriskowal · 2025-02-25T19:56:43Z

Description

In support of #9674, this change adds a regression test for npm install to increase our confidence that changes to build.js support both local development and eventual installation from npm.

Security Considerations

None.

Scaling Considerations

A design question for this is whether to use a temporary directory for the extracted package. I chose to use the default package since this speeds up non-initial test runs. Since tar xf overlays the package over the previous version, there’s a small chance that the deletion of a sensitive file might go unnoticed locally, but not in CI.

Documentation Considerations

None.

Testing Considerations

It’s a test, Bob.

Upgrade Considerations

None.

cloudflare-workers-and-pages · 2025-02-25T19:57:15Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`a0c13da`
Status:	✅ Deploy successful!
Preview URL:	https://fb74536a.agoric-sdk.pages.dev
Branch Preview URL:	https://kriskowal-xsnap-install-test.agoric-sdk.pages.dev

View logs

kriskowal · 2025-02-25T19:59:38Z

packages/xsnap/src/build.js

@@ -29,7 +29,7 @@ const ModdableSDK = {
 *   spawn: typeof import('child_process').spawn,
 * }} io
 */
-function makeCLI(command, { spawn }) {
+export function makeCLI(command, { spawn }) {


This is an unabashèd shortcut, punting on the effort and thought that should go into extracting a utility for use in other packages or committing to a shared dependency that replaces the functionality. Whatever form that takes, we need to consider that the dependency will be a full dependency, not a devDependency, and assess risk accordingly. Given that there are intersecting changes in concurrent PRs, punt punt punt.

gibson042

I'm not worried about the speed of this test; following good practices (including cleanup) is more important. So 👍 to using a temp dir.

gibson042 · 2025-02-26T15:30:18Z

packages/xsnap/test/install.test.js

+  const npm = makeCLI('npm', { spawn });
+  const tar = makeCLI('tar', { spawn });
+  const npmRes = await npm.pipe(['pack', '--json']);
+  const {
+    0: { filename },
+  } = JSON.parse(npmRes);
+  await tar.run(['xf', filename]);
+  await npm.run(['install'], { cwd: 'package' });


Rather than extracting makeCLI, I recommend following userland's use of execa for testing as seen in e.g. fast-usdc.

Suggested change

const npm = makeCLI('npm', { spawn });

const tar = makeCLI('tar', { spawn });

const npmRes = await npm.pipe(['pack', '--json']);

const {

0: { filename },

} = JSON.parse(npmRes);

await tar.run(['xf', filename]);

await npm.run(['install'], { cwd: 'package' });

const { stdout: npmRes } = await execa`npm pack --json`;

const {

0: { filename },

} = JSON.parse(npmRes);

await execa`tar xf ${filename}`;

await execa({ cwd: 'package' })`npm install`;

gibson042 · 2025-02-26T15:30:58Z

packages/xsnap/test/install.test.js

+  const {
+    0: { filename },
+  } = JSON.parse(npmRes);


Making this more idiomatic would improve readability.

Suggested change

const {

0: { filename },

} = JSON.parse(npmRes);

const [{ filename }] = JSON.parse(npmRes);

mhofman

I'm confused how this test does not leave junk behind that the "clean worktree" CI test would trip on.

kriskowal · 2025-02-27T22:59:48Z

I'm confused how this test does not leave junk behind that the "clean worktree" CI test would trip on.

The clean worktree CI test occurs before this test generates the junk. Unless I’m confused. But, I will be moving the junk to a temporary directory for idempotence.

mhofman · 2025-02-27T23:34:39Z

The clean worktree CI test occurs before this test generates the junk. Unless I’m confused. But, I will be moving the junk to a temporary directory for idempotence.

#9804 was supposed to have fixed #5140. If that's not the case, I'd like to understand how it failed.

The test in which cd packages/xsnap && yarn test ran show it didn't find any dirty files. Maybe we're git ignoring the junk already?

kriskowal · 2025-02-28T00:14:01Z

#9804 was supposed to have fixed #5140. If that's not the case, I'd like to understand how it failed.

I see. I did not know we did our dirty check after running tests. The reason it succeeded is that this change, for unrelated reasons, adds package to packages/xsnap/.gitignore, which is the conventional name of the directory inside the tarball that npm pack generates, and the build script in there only creates files inside that package directory tree.

mhofman · 2025-02-28T01:03:56Z

adds package to packages/xsnap/.gitignore

Ah and the tgz is excluded in the root gitignore:

agoric-sdk/.gitignore

Lines 42 to 43 in a27f724

    
           # Output of 'npm pack' 
        
           *.tgz

mergify · 2025-03-03T20:41:02Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify · 2025-03-03T22:22:15Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify · 2025-03-03T23:17:04Z

This pull request has been removed from the queue for the following reason: pull request manually updated.

The pull request #11051 has been manually updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

mergify · 2025-03-04T00:22:07Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Ref #9614 Ref #9618 Blocked on #11051 ## Description Simplifies and documents functions. See individual commits for details. ### Security Considerations None. ### Scaling Considerations n/a ### Documentation Considerations Includes JSDoc improvements. ### Testing Considerations None. ### Upgrade Considerations n/a

kriskowal requested a review from a team as a code owner February 25, 2025 19:56

kriskowal requested review from AgoricTriage and gibson042 February 25, 2025 19:56

kriskowal commented Feb 25, 2025

View reviewed changes

gibson042 approved these changes Feb 26, 2025

View reviewed changes

mhofman reviewed Feb 27, 2025

View reviewed changes

gibson042 mentioned this pull request Feb 28, 2025

refactor(xsnap): Cleanup build.js #9674

Merged

kriskowal force-pushed the kriskowal-xsnap-install-test branch 2 times, most recently from f7a079f to b26f5c5 Compare March 1, 2025 00:56

kriskowal added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Mar 1, 2025

kriskowal force-pushed the kriskowal-xsnap-install-test branch 2 times, most recently from f0a22b5 to fe9d407 Compare March 1, 2025 01:48

kriskowal added the automerge:rebase Automatically rebase updates, then merge label Mar 3, 2025

kriskowal force-pushed the kriskowal-xsnap-install-test branch 2 times, most recently from 7ea2117 to 667a915 Compare March 3, 2025 19:56

kriskowal force-pushed the kriskowal-xsnap-install-test branch 4 times, most recently from 68c388e to 5117d8e Compare March 3, 2025 23:16

test(xsnap): Verify npm install build process

a0c13da

kriskowal force-pushed the kriskowal-xsnap-install-test branch from 5117d8e to a0c13da Compare March 3, 2025 23:36

mergify bot merged commit 34b85df into master Mar 4, 2025
84 checks passed

mergify bot deleted the kriskowal-xsnap-install-test branch March 4, 2025 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(xsnap): Verify npm install build process #11051

test(xsnap): Verify npm install build process #11051

kriskowal commented Feb 25, 2025

cloudflare-workers-and-pages bot commented Feb 25, 2025 •

edited

Loading

kriskowal Feb 25, 2025

gibson042 left a comment

gibson042 Feb 26, 2025

gibson042 Feb 26, 2025

mhofman left a comment

kriskowal commented Feb 27, 2025

mhofman commented Feb 27, 2025 •

edited

Loading

kriskowal commented Feb 28, 2025

mhofman commented Feb 28, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 4, 2025

test(xsnap): Verify npm install build process #11051

test(xsnap): Verify npm install build process #11051

Conversation

kriskowal commented Feb 25, 2025

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Feb 25, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

kriskowal Feb 25, 2025

Choose a reason for hiding this comment

gibson042 left a comment

Choose a reason for hiding this comment

gibson042 Feb 26, 2025

Choose a reason for hiding this comment

gibson042 Feb 26, 2025

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

kriskowal commented Feb 27, 2025

mhofman commented Feb 27, 2025 • edited Loading

kriskowal commented Feb 28, 2025

mhofman commented Feb 28, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 3, 2025

mergify bot commented Mar 4, 2025

cloudflare-workers-and-pages bot commented Feb 25, 2025 •

edited

Loading

mhofman commented Feb 27, 2025 •

edited

Loading